-
-
Notifications
You must be signed in to change notification settings - Fork 231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rz passthrough cover #56
Conversation
Final note: I'm really interested in which compiler options causes the re-compilation of the abstract code to fail? I had a similar case earlier when developing meck but can't remember exactly which options caused the trouble. I'm talking to some people on the OTP team discussing possible solutions (some of which might render this whole beam dance unnecessary, hopefully). |
In regards to your final note I'm going to assume you are asking about commit The problem is that rebar, when cover is enabled, cover compiles modules with Do you follow? This was one of the hardest parts for me to figure out so perhaps I got something wrong and there is an easier way. |
Okay, let's try to structure this:
And, when unloading:
Did I get this right? :-) |
To summarize the paths taken by meck. In the normal case (no passthrough cover): Loading
In the passthrough cover case: Loading
As far as I can understand, the _save beam_ step is only to please rebar? |
Some thoughts:
|
You understand the sequence correct in your diagrams above. The save beam step has nothing to do with pleasing rebar. Go ahead and comment out the line in
If you look at I think for this to work more smoothly someone needs to patch cover to work with binaries. In the mean time I'm not sure how else to make this work. As for your last 3 points
|
Is there some way to call a function even if it's not exported? If so then we could call |
No, it's not possible to call unexported functions. Messing with cover and it's abstract code I think is the last option in case everything else fails. |
So with the last commit I was able to avoid saving the beam to disk. However, I had to keep it in state. No matter how hard I tried I can't find a way to load the binary from the code server without saving it to disk. Even calling Anyways, even with my change things still feel a little sloppy but I wanted to share what I had thus far. I still have some other changes I want to make to try to reduce some of the code needed. I also still need to make the change to default to pass_cover and add option to turn it off. |
Yeah, storing it in state makes more sense. I have plans to create a branch where I'll research the different ways of dealing with compiler options and module binaries. Right now I'm re-vamping the meck homepage a bit so that's why I don't have as much time for this as I want at the moment. |
I still want to make the adjustment around the original compile options (and remove the |
Ah, good stuff. I'll get back to you with a review as soon as I can. :-) |
FWIW, this worked for me in real tests in Riak code :) |
Also, I'm happy to rebase this into one commit. I initially planned to do that but just forgot. |
Do you know the build / test status between commits? If they all build and pass all tests, I think it's better with smaller commits with sensible messages. :-) |
No, these commits were me feeling my way through it. I see this as one logical change. All the commits were to achieve one goal. If I were to rebase them into smaller, but multiple commits it would feel arbitrary to me. |
So are they fine as they are or should we merge them into one big commit? |
Did an interactive rebase on latest master, squashed to one commit, updated the comment a bit, and did a force push. Hope you like it :) |
I have to admit that I was initially put off by the sheer size of the commit, but since the majority of the code deals with hacking cover to do what we need, I think it's okay. I'd prefer if that code (which is fairly generic), is in a new module |
And great work with all the patches btw. Will definately pull this in when it's ready! :-) |
I think I addressed most of your concerns. Just a few comments for you to read for things I haven't changed. Once I get your blessing I'm going to squash these new commits back into the first one and then you can merge :) |
Looks good so far! Let's squash it and I'll take a final look at that. |
How about now? |
This time I'm not going to bother squashing until you say the code looks good :) |
Ok, looks good. I'll pull it once it's squashed. :-) |
If a module with cover instrumentation is mocked then make sure to instrument any passtrhough calls. This way coverage analysis is still available for passtrhough calls. This functionality can be disabled via the `no_passthrough_cover` option. Implementation Details ---------------------- This is coded for the specific use case of running eunit tests from rebar with coverage analysis enabled. Previously, if just some of the functions in a src module were mocked then all coverage analyis on said module, while it was mocked, was lost. 1. check if module is instrumented for coverage 2. compile `<name>_meck_original` (thus known as `OriginalMod`) and then cover compile it 3. let meck do it's thing 4. during unload/termination of mocked module make sure to first export the cover data collected on `OriginalMod`, after exporting modify the data to use the real modules name (e.g. `foo_meck_original` -> `foo`), this way the data can be imported and counted against `foo` 5. let meck do it's usual cleanup 6. during restore import the `OriginalMod` coverage data Tricks are played with cover's BEAM code so that private functions can be called. This was done to avoid creating temporary files and copy/pasting code from cover.
Done |
Record cover data on passtrhough calls
Awesome work! Thanks a lot! |
EDIT: Collecting cover data on a mocked module is the default now. To disable the feature use
no_passthrough_cover
option.This patch adds a
pass_cover
option which will allow coverage analysis to be collected forpassthrough
calls.Feel free to nitpick the PR for anything. No ego here. Just want to see this ability in meck.
Technically, I don't think the last commit
6a7ad20a
is needed, but I thought I'd leave it in and let you decide. I spent a lot of time pulling my hair trying to understand how the code server, rebar, cover, and meck all interact with each other. I'm pretty happy with this solution. I'd like it even better if I didn't have to manually tease apart the cover data but I didn't see another way.